[ASRPass] Wrap all the global symbols into a module #1491

Thirumalai-Shaktivel · 2023-02-02T09:23:56Z

This PR aims to move all the symbols from the global scope (except main_program) into the Module (_global_scope).
Now the TranslationUnit contains only two symbols: Module (_global_scope) and a Program (main_program).
For more details and discussion: #1384

certik · 2023-02-02T14:49:08Z

Yes, this is good. Once this passes tests, please ping me.

certik · 2023-02-19T03:33:24Z

src/libasr/codegen/asr_to_c.cpp

+                visit_symbol(*struct_sym);
+                array_types_decls += src;
+            }
+        }


Why do we need special handling of this global_symbols module? I was hoping that since it's just a regular module, it would not need any special handling like this.

Yup, here in C backend, I think the visit_module is not yet implemented, except for the lfortran_intrinsics module.

Correct. Probably this can be removed. I guess you can just do if(ASR::is_a<ASR::Module_t>(*item.second)) and call visit_Module(*ASR::down_cast<ASR::Module_t>(item.second)) inside the if block by creating a new loop similar to the following,

lpython/src/libasr/codegen/asr_to_c.cpp

Lines 613 to 623 in 8f62ac4

for (auto &item : x.m_global_scope->get_scope()) {

if (ASR::is_a<ASR::Variable_t>(*item.second)) {

ASR::Variable_t *v = ASR::down_cast<ASR::Variable_t>(item.second);

unit_src_tmp = convert_variable_decl(*v);

unit_src += unit_src_tmp;

if(unit_src_tmp.size() > 0 && (!ASR::is_a<ASR::Const_t>(*v->m_type) ||

v->m_intent == ASRUtils::intent_return_var )) {

unit_src += ";\n";

}

}

}

Please comment here if you face any blocker or issues while following this approach.

czgdp1807 · 2023-02-19T14:41:16Z

src/libasr/asr_scopes.cpp

+                throw LCompilersException("Moving the symbol:`" + a.first +
+                    "` from global scope is not implemented yet");
+            };
+        }


This isn't scalable. I would do it by writing a visitor in asdl_cpp.py. Will you be able to try the class design and implement it? Give it a try for a couple of days, if not then we will merge this with a TODO.

Added this in TODO

certik · 2023-02-22T18:33:16Z

src/libasr/pass/array_op.cpp

@@ -182,6 +182,38 @@ class ArrayOpVisitor : public PassUtils::PassVisitor<ArrayOpVisitor>
        }
    }

+    void visit_Module(const ASR::Module_t &x) {
+        if (strcmp(x.m_name, "_global_symbols") == 0) {
+            // FIXME: this is a hack, we need to pass in a non-const `x`,


try removing this if check. If it does not work, let's figure out the real bug why it doesn't work.

certik · 2023-02-22T18:33:45Z

src/libasr/pass/subroutine_from_function.cpp

@@ -94,6 +94,38 @@ class CreateFunctionFromSubroutine: public PassUtils::PassVisitor<CreateFunction
            }
        }

+        void visit_Module(const ASR::Module_t &x) {
+            if (strcmp(x.m_name, "_global_symbols") == 0) {
+                // FIXME: this is a hack, we need to pass in a non-const `x`,


certik · 2023-02-22T18:35:39Z

Let's see if we can just construct the new module, but then treat it like any other module, no special treatment. That would be the cleanest design.

Added this in TODO

certik · 2023-02-25T15:01:02Z

@Thirumalai-Shaktivel let me know when this is ready for review.

Thirumalai-Shaktivel · 2023-02-25T15:01:48Z

Yup, I think I made the required changes. This is ready for review.

certik · 2023-02-25T15:57:53Z

Thanks! This requires a thorough review, I'll try to do it soon. It looks good at first sight though.

@czgdp1807 can you also please review this?

czgdp1807 · 2023-02-26T08:14:57Z

integration_tests/CMakeLists.txt

@@ -427,4 +427,5 @@ RUN(NAME comp_01 LABELS cpython llvm wasm wasm_x64)
 RUN(NAME bit_operations_i32 LABELS cpython llvm wasm wasm_x64)
 RUN(NAME bit_operations_i64 LABELS cpython llvm wasm)

-RUN(NAME test_argv_01    LABELS llvm) # TODO: Test using CPython
+RUN(NAME test_argv_01      LABELS llvm) # TODO: Test using CPython
+RUN(NAME global_syms_01    LABELS cpython)


Why not llvm and c here?

In LLVM, the global lists are not yet handled. After merging this I will start working on it.
C: I didn't try it. How do I verify this works in C?
Using lpython integration_tests/global_syms_01.py --show-c?

In LLVM, the global lists are not yet handled. After merging this I will start working on it.

Any rough idea on how that will be managed. I think LLVM doesn't allow builder->CreateAlloca in global scope. Worth researching approaches (implementation in LLVM is not at all required just a rough idea that there is a possibility to make it work in LLVM) once this before merging this because the success of this might rely on the possibility of actually using it in LLVM.

One approach in my mind is to create void* typed global variables in the global scope and then bitcast them into list LLVM type inside LLVM functions. That way any manipulation will happen inside LLVM functions.

Other approach is to see how clang++ does it for global variables like std::vector in C++. For example the below code,

#include <iostream> #include <vector> std::vector<int> vec; int main() { return 0; }

clang++'s LLVM output,

; ModuleID = '/Users/czgdp1807/lpython_project/debug.cpp' source_filename = "/Users/czgdp1807/lpython_project/debug.cpp" target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128" target triple = "arm64-apple-macosx13.0.0" %"class.std::__1::vector" = type { %"class.std::__1::__vector_base" } %"class.std::__1::__vector_base" = type { i32*, i32*, %"class.std::__1::__compressed_pair" } %"class.std::__1::__compressed_pair" = type { %"struct.std::__1::__compressed_pair_elem" } %"struct.std::__1::__compressed_pair_elem" = type { i32* } %"struct.std::__1::nullptr_t" = type { i8* } %"struct.std::__1::__default_init_tag" = type { i8 } %"class.std::__1::__vector_base_common" = type { i8 } %"struct.std::__1::__compressed_pair_elem.0" = type { i8 } %"class.std::__1::allocator" = type { i8 } %"struct.std::__1::__non_trivial_if" = type { i8 } @vec = global %"class.std::__1::vector" zeroinitializer, align 8 @__dso_handle = external hidden global i8 @llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I_debug.cpp, i8* null }] ; Function Attrs: noinline ssp uwtable define internal void @__cxx_global_var_init() #0 section "__TEXT,__StaticInit,regular,pure_instructions" { %1 = call %"class.std::__1::vector"* @_ZNSt3__16vectorIiNS_9allocatorIiEEEC1Ev(%"class.std::__1::vector"* @vec) %2 = call i32 @__cxa_atexit(void (i8*)* bitcast (%"class.std::__1::vector"* (%"class.std::__1::vector"*)* @_ZNSt3__16vectorIiNS_9allocatorIiEEED1Ev to void (i8*)*), i8* bitcast (%"class.std::__1::vector"* @vec to i8*), i8* @__dso_handle) #2 ret void }

It can be seen that it creates global variables but initialises them inside a function. So you can do something similar for lists as well. Also, worth noting that we will have to create a function something like global_module_function in LLVM and then call inside main. But that should be doable.

C: I didn't try it. How do I verify this works in C?
Using lpython integration_tests/global_syms_01.py --show-c?

If LLVM is not enabled then C is not worth enabling. LLVM should always work with a given ASR design then other backends will automatically follow.

I am approving it as I think this is clean enough at the ASR as well as LLVM/C level.

@czgdp1807 the question here is how to handle module variables, as well as program variables. They are scoped to the module or program, but are essentially global. WASM and x86 have the same issue (@Shaikh-Ubaid and I discussed this a few times). It seems one must allocate the global variable (using the platform dependent mechanism), and then initialize it from the main function as needed.

So seems like global statements are already being wrapped into a main function in the ASR itself? Well I would like to see what we will need to do when we enable llvm for this test. Let’s merge it for now (I have approved earlier today).

Let me review this first.

certik

I think that this is fine. I reviewed it and didn't find anything major.

Thirumalai-Shaktivel · 2023-02-28T06:07:07Z

Thanks for the approvals.

src/libasr/asr_verify.cpp

* Move all the symbols from the global scope into the `_global_symbols` module * After applying this pass, there will only be two symbols in TranslationUnit, .ie., `Module` (_global_symbols) and `Program` (main_program) * `main_program` calls the `_lpython_main_program` (which contains the global statements) function from the module.

certik

I think this is fine to merge and iterate on later.

Thirumalai-Shaktivel added the asr_pass ASR pass related changes label Feb 2, 2023

Thirumalai-Shaktivel force-pushed the global_symbols branch 4 times, most recently from 76d3891 to 5923b99 Compare February 10, 2023 13:50

Thirumalai-Shaktivel force-pushed the global_symbols branch 3 times, most recently from 99bd649 to 0003a20 Compare February 18, 2023 15:36

Thirumalai-Shaktivel marked this pull request as ready for review February 18, 2023 15:46

Thirumalai-Shaktivel requested a review from certik February 18, 2023 16:20

certik reviewed Feb 19, 2023

View reviewed changes

czgdp1807 previously requested changes Feb 19, 2023

View reviewed changes

czgdp1807 marked this pull request as draft February 22, 2023 14:51

certik reviewed Feb 22, 2023

View reviewed changes

Thirumalai-Shaktivel force-pushed the global_symbols branch from 0003a20 to 6744ac5 Compare February 23, 2023 05:43

Thirumalai-Shaktivel marked this pull request as ready for review February 24, 2023 07:21

Thirumalai-Shaktivel force-pushed the global_symbols branch from b26f1ae to d374871 Compare February 25, 2023 06:44

czgdp1807 reviewed Feb 26, 2023

View reviewed changes

czgdp1807 approved these changes Feb 26, 2023

View reviewed changes

certik approved these changes Feb 28, 2023

View reviewed changes

certik reviewed Feb 28, 2023

View reviewed changes

src/libasr/asr_verify.cpp Outdated Show resolved Hide resolved

Thirumalai-Shaktivel force-pushed the global_symbols branch from d374871 to 458497f Compare February 28, 2023 06:22

Thirumalai-Shaktivel added 5 commits February 28, 2023 12:17

[C, x86] Add Visit Module to visit the module symbols

39aed26

[LLVM] visit EnumType in the Module scope

b0bd7b8

[ASRPass] Add visit Module in passes

ccd3cf1

Add tests and update the refs.

092a46e

Thirumalai-Shaktivel force-pushed the global_symbols branch from 3642151 to 092a46e Compare February 28, 2023 06:48

certik approved these changes Feb 28, 2023

View reviewed changes

Thirumalai-Shaktivel merged commit 12d9574 into lcompilers:main Mar 1, 2023

Thirumalai-Shaktivel deleted the global_symbols branch March 1, 2023 02:56

Thirumalai-Shaktivel mentioned this pull request Mar 7, 2023

Initial support for global Lists #1384

Merged

Thirumalai-Shaktivel mentioned this pull request Mar 20, 2023

MSVC gives different outputs compared to WSL build, Linux/Mac OS ? #1593

Open

	for (auto &item : x.m_global_scope->get_scope()) {
	if (ASR::is_a<ASR::Variable_t>(*item.second)) {
	ASR::Variable_t *v = ASR::down_cast<ASR::Variable_t>(item.second);
	unit_src_tmp = convert_variable_decl(*v);
	unit_src += unit_src_tmp;
	if(unit_src_tmp.size() > 0 && (!ASR::is_a<ASR::Const_t>(*v->m_type) \|\|
	v->m_intent == ASRUtils::intent_return_var )) {
	unit_src += ";\n";
	}
	}
	}

[ASRPass] Wrap all the global symbols into a module #1491

[ASRPass] Wrap all the global symbols into a module #1491

Uh oh!

Conversation

Thirumalai-Shaktivel commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Feb 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Feb 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik commented Feb 22, 2023

Uh oh!

certik commented Feb 25, 2023

Uh oh!

Thirumalai-Shaktivel commented Feb 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Feb 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel commented Feb 28, 2023

Uh oh!

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Feb 2, 2023 •

edited

Loading

Thirumalai-Shaktivel Feb 19, 2023 •

edited

Loading

Thirumalai-Shaktivel commented Feb 25, 2023 •

edited

Loading

certik commented Feb 25, 2023 •

edited

Loading